Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf(hmr): skip traversed modules when checking circular imports #15034

Merged
merged 2 commits into from
Nov 21, 2023
Merged

perf(hmr): skip traversed modules when checking circular imports #15034

merged 2 commits into from
Nov 21, 2023

Conversation

skovhus
Copy link
Contributor

@skovhus skovhus commented Nov 19, 2023 •
edited
Loading

Description

This PR is fixing graph traversal issue where long module graph seems to be explored repetitively, at least the HMR reloading is super slow and can sometimes result in the server becoming irresponsive. It is likely related to the codebase having a lot of barrel files and circular imports.

Additional context

I just tried 5.0 out an a large codebase with a lot of circular imports and barrel files. Unfortunately we often end up on cases where Vite becomes irresponsive when changing certain files.

The regression is related to #14867.

I could come up with a test case that reproduces this. @ArnaudBarre do you have an idea on how to do that?

The solution was suggested by @ArnaudBarre here.

Performance improvement: most importantly the server is now responsive again, but for some files that took 14 seconds to determine a circular import (vite 5.0.0), it now takes ~10 ms.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

@skovhus skovhus mentioned this pull request Nov 19, 2023
Merged
9 tasks
@skovhus skovhus changed the title fix(hmr): prevent infinite recursion fix(hmr): prevent infinite graph traversal Nov 19, 2023
@ArnaudBarre
Copy link
Member

I spent sometimes looking at the code yesterday and I don't see how it could end up in infinite recursion. My only thought was just that it was slow to explore the graph, but even with if a big graph it should not freeze your computer, just make HMR really slow. So there is probably something I missing, but if this change works and the test are passing I think this is still a good change. Maybe @bluwy will understand better what is happening.

ArnaudBarre
ArnaudBarre reviewed Nov 19, 2023
View reviewed changes
Copy link
Member

@ArnaudBarre ArnaudBarre left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing it and making the PR!
Edit: You forgot the line to pass the set in recursion

packages/vite/src/node/server/hmr.ts Outdated Show resolved Hide resolved
@skovhus skovhus force-pushed the kenneth/fix-infinite-recursion-in-circular-check branch from 7408fc1 to 3cc24a0 Compare November 19, 2023 14:50
@skovhus
Copy link
Contributor Author

skovhus commented Nov 19, 2023

I spent sometimes looking at the code yesterday and I don't see how it could end up in infinite recursion. My only thought was just that it was slow to explore the graph, but even with if a big graph it should not freeze your computer, just make HMR really slow. So there is probably something I missing, but if this change works and the test are passing I think this is still a good change.

Yeah I also tried to think what could go wrong. Also tried to make a set of the traversed chains urls and checked if the current chain was in there without luck. After introducing traversedModules I also tried not returning in case node was included, but then we run out of memory (which signals that we are in an infinite graph traversal).

Edit: You forgot the line to pass the set in recursion

Fixed. :)

bluwy
bluwy reviewed Nov 20, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really think of a case where a recursion could happen too, but I can see traversedModule being a nice perf improvement. For cases like:

graph TD
    A ---> B
    A ---> C
    B ---> D
    C ---> D
    D ---> E
Loading

If D already failed once, we can early out and not process it anymore (since we also do a depth-first search). Perhaps the lack of this was causing a huge memory pressure and not necessarily a recursion 🤔

packages/vite/src/node/server/hmr.ts Outdated Show resolved Hide resolved
@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) performance Performance related enhancement feat: hmr labels Nov 20, 2023
@skovhus
Copy link
Contributor Author

skovhus commented Nov 20, 2023

I can't really think of a case where a recursion could happen too, but I can see traversedModule being a nice perf improvement.

I just verified the performance improvement. Before isNodeWithinCircularImports took 14 seconds to determine a circular import on one of our files, after this PR it takes 10 ms. So a nice ~1400X speed up. :)

@skovhus skovhus changed the title fix(hmr): prevent infinite graph traversal fix(hmr): improve performance and prevent infinite graph traversal Nov 20, 2023
@skovhus skovhus force-pushed the kenneth/fix-infinite-recursion-in-circular-check branch from bdb5362 to c9233eb Compare November 20, 2023 08:38
@skovhus skovhus requested a review from bluwy November 20, 2023 17:11
bluwy
bluwy approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bluwy bluwy changed the title fix(hmr): improve performance and prevent infinite graph traversal perf(hmr): skip traversed modules when checking circular imports Nov 21, 2023
@bluwy bluwy merged commit 41e437f into vitejs:main Nov 21, 2023
13 checks passed
@skovhus skovhus deleted the kenneth/fix-infinite-recursion-in-circular-check branch November 21, 2023 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArnaudBarre ArnaudBarre ArnaudBarre left review comments

@bluwy bluwy bluwy approved these changes

Assignees
No one assigned
Labels
feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority) performance Performance related enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skovhus @ArnaudBarre @bluwy